-
-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add musl prebuild #264
feat: add musl prebuild #264
Conversation
Co-authored-by: funniray <[email protected]> Co-authored-by: Brad Marsden <[email protected]>
There appears to be an issue with glibc builds still which we're working through, I will mark this ready for review once we've got these changes ready. |
Reverts back to using ubuntu-20.04 for builds to keep in-line with existing worker. Uses debian instead of ubuntu for sysroot for newer version of OpenSSL. Runs all tests on all platforms to ensure everything works, rather than just x64. Co-authored-by: funniray <[email protected]> Co-authored-by: Brad Marsden <[email protected]>
This should now be fixed and ready for review! |
Thank you very much for the PR. Could you please explain me |
And if I compare the binary sizes they are different. https://github.com/SilentBot1/node-datachannel/releases/tag/v0.9.1 |
Different programs can't agree on what to call each architecture.
Alpine and Debian disagree on the architecture for the triple for armv7. Debian calls it arm, while Alpine calls it armv7. This is why there's an override on debian arm[v7] builds to set the triple's target to just
libssl.a on alpine is 6.3MB as libssl is linked statically, I'd assume that's why the filesize is larger on musl vs debian |
Thank you for the explanation. I think it will be good first to merge with another branch. So we can test it better, and then merge it with the master. I created a new branch here |
Sure, I've repointed this PR to the new branch you've mentioned. Please note, that we probably will want to bump the version for this branch once this is merged (or give it a postfix like -dev / -testing) so it doesn't interfere with the currently published release and its prebuilds. Feel free to merge whenever you are ready. |
I have created a release here. https://github.com/murat-dogan/node-datachannel/releases/tag/v0.10.0-dev |
For Linux x64 ; Less is better of course, but I wonder why. |
I checked and found out
I'm honestly unsure what could be causing the smaller builds. I don't have enough experience with C/C++ to know how to debug why the files are larger any further. As far as I'm aware however, the only difference the compilers would see is the openssl version |
The difference is pthread linking Current
New
|
Hey there, I don't seem to see the same results as you. Are you sure you're checking the right file?
old-linux.node was from 0.9.2-x64 |
Yep, you are right. I was comparing with local built version. Then, I am again at the beginning :) |
Found a diff by running command Current
new
Difference is on I have never used this lib |
Definitely a difference, but I'm unsure if it's the real difference that's causing the filesize differences. After extracting, the new one is almost 2MB smaller. The sections you mentioned only make up 233 bytes in total. It's possible that the tool is storing more data elsewhere in the file, but I'm unsure if it's the actual culprit. |
From readelf command I mostly see SSL Lib differences besides If all tests are passed we can merge I think. |
I also compared the performance, no difference |
Since the tests pass and the openssl 1.1 version is newer than the one in prior builds, I see no reason to not go through with merging it |
Likewise, everything has worked as expected in all my testing, so this looks good to me. |
I think also same. |
I have just released v0.10.0 |
Awesome, thanks :) As a quick note @murat-dogan, it appears the |
It seems, also in 2-3 months |
I updated Mac-x64 CI file also to OpenSSL 1.1.1w |
And unfortunately, I don't have a Mac. |
I'll test this now and let you know, I have both an M2 and arm64 mac available to test on. |
For arm64 I am still checking |
Tried to update Mac M1 arm64 to also 1.1.1w but since there is no Mac M1 native machine on github actions, now I could not. Anyway with 1.1.1t now ready to test. |
The pre-builds worked on both devices (arm64, amd64), so they look good to me :) |
In this PR, we add musl prebuilds of
libdatachannel
tonode-datachannel
to allow users on musl-based systems (Alpine, Kiss Linux) to usenode-datachannel
without needing to buildlibdatachannel
locally.This is done by generating sysroots locally for glibc (ubuntu) and musl (alpine) and using these to prebuild
libdatachannel
.An example of these builds can be seen here: https://github.com/SilentBot1/node-datachannel/releases/tag/v0.9.1
And this can be tested locally by doing
npm i https://github.com/SilentBot1/node-datachannel.git